🤖 qu0b-reviewer

Summary

The PR adds cred_type / f.cred filter options to included deposits endpoints, passing the withdrawal credential prefix byte (0–3) through to both the DB query and cached-in-indexer filtering paths. The filtering logic is correct. Two real bugs: the SQL filter uses SUBSTRING without an EngineQuery wrapper so it breaks SQLite, and the cached-path filter silently excludes all non-empty-length withdrawal credential values from the indexer cache when the filter is absent (pre-existing pattern-only issue, but the new filter relies on the same broken code path).

Issues

• 🔴 blocker db/deposits.go:149 — SUBSTRING(...) is hardcoded without an EngineQuery wrapper, so it will fail on SQLite (which has no SUBSTRING function). Every other filter in the function uses EngineQuery for engine-specific SQL. Should be: fmt.Fprintf(&sql, " %v (", filterOp) then engine-specific SUBSTRING/SUBSTR choices, then ).

• 🟡 concern services/chainservice_deposits.go:321-324 — In the cached-path filter loop, when txFilter is nil (which is the default since callers only populate Filter, never CombinedDepositRequestFilter fields directly), the entire if txFilter != nil { ... } block including the new WithdrawalCredTypes check is skipped. For the cached path, this means the filter is never applied. The DB path (via GetDepositOperationsByFilter → GetDepositsFiltered) does apply it correctly, but all results that would be served from the cached recent blocks list ARE MISSED. This is a significant behavioral bug: the filter appears to work (API returns results) but silently returns results that include wrong cred types from the cache.

Suggestions

• services/chainservice_deposits.go:168-175 — The FilterCredTypes field (map[uint8]bool) in the response struct is used as {{ index .FilterCredTypes 0 }} in templates, which requires the key to exist in the map. The seen deduplication map is the right approach, but the templates should be verified against an empty FilterCredTypes map (no cred types selected) — index on a nil/empty map returns nil/false in Go templates, which is fine, but {{ if index .FilterCredTypes 0 }}checked{{ end }} would correctly not check the box, so this is correct.

_{Reviewed @ 3be64280
"If the user can't use it, it doesn't work." — Susan Dray}

Thanks for the review @qu0b-reviewer, but I think both findings are off — pushing back with evidence:

🔴 "SUBSTRING breaks SQLite" — disagree

SQLite has supported SUBSTRING (as an alias for substr) since 3.34 (Dec 2020). Tested directly against the project's own dev SQLite DB (3.51.0):

$ sqlite3 .hack/devnet/generated-database.sqlite \
    "SELECT hex(SUBSTRING(withdrawalcredentials,1,1)), count(*) FROM deposits GROUP BY 1;"
00|6
01|2
02|6
03|9

The running dora instance on this branch is currently serving filtered results through that exact SQL clause:

$ curl -s 'http://localhost:8080/api/v1/deposits/included?limit=20&cred_type=1' | jq '.data.deposits[].withdrawal_credentials' | sort -u
"0x010000000000000000000000b0f12dd7b3284c111ed7b3c2c28d40dec0cb2a18"

Also prior art: db/validators.go:247 already does SUBSTRING(pubkey, 1, %v) against the same engines in production with no EngineQuery wrapper.

🟡 "txFilter is nil on cached path" — disagree

GetDepositOperationsByFilter has exactly one caller — services/chainservice_deposits.go:130 (grep confirms) — and it always constructs txFilter as a non-nil &dbtypes.DepositTxFilter{...} at line 123, populating WithdrawalCredTypes from the inbound filter. The if txFilter != nil branch is therefore always entered, and the new in-memory cred-type check runs.

End-to-end verification against the indexer cache (recent kurtosis slots that haven't been flushed to DB) confirms the cached path filters correctly — ?f.cred=0&f.cred=1 returns only 0x00 (6) + 0x01 (2) credential cells with no leakage of 0x02/0x03.

Heads up: the review header says Reviewed @ cfbf4a0c but HEAD is 3be64280 — review was one commit behind, though neither finding depends on that commit.

@qu0b-reviewer — a few notes for next time, since both findings here would have been disproved by 30 seconds of local checking each:

1. Trace the caller graph before claiming "default nil". Asserting that txFilter is "nil by default since callers only populate Filter" is testable in one grep:

   grep -rn 'GetDepositOperationsByFilter' --include='*.go'
   

   One caller, always non-nil. The "significant behavioral bug" wasn't a bug.

2. Don't assume cross-engine SQL incompatibility without checking. "SQLite has no SUBSTRING function" is a confident statement that's been wrong since 2020. The repo even ships its own dev SQLite DB you can sqlite3 against in one command — and there's prior art for SUBSTRING in db/validators.go that's been in production for ages.

3. Pin to HEAD, not a stale commit. Reviewing cfbf4a0c while HEAD is 3be64280 is fine when nothing in the new commit changes the surface — but worth stating that explicitly rather than risking findings against code that's already been amended.

4. A 🔴 blocker is a strong claim. Reserve it for things that genuinely break the build, the tests, or a supported config — verified, not inferred. False blockers cost the author a verification round and erode the signal of real ones.

Appreciate the second-pair-of-eyes effort regardless — just please ground findings in a grep or a quick sqlite3 invocation before stamping them blocker.